feat: add microshift-prow-jobs plugin #78
Conversation
openshift-merge-robot
added
needs-rebase
agullon
changed the title
|
/retest |
|
/test images |
stbenjam
left a comment
stbenjam
left a comment
There was a problem hiding this comment.
Nice contribution thanks, a few comments below
There was a problem hiding this comment.
Could it just be part of prow-jobs? microshift-prow-jobs feels a little too specialized. Either a microshift plugin, or contribute it to the prow-jobs plugin will make it more discoverable for other folks who might be debugging various CI jobs
There was a problem hiding this comment.
I think there's value on a more specialized prow-job plugin only for microshift because MicroShift tests are not based on ginkgo test suites but on Robot Framework.
There was a problem hiding this comment.
Maybe just a dedicated microshift plugin then? /microshift-prow-job:analyze-test-scenario is quite long
There was a problem hiding this comment.
do you mean renaming the plugin from microshift-prow-job to microshift ?
There was a problem hiding this comment.
Yes, microshift-prow-job is too long and too specific IMHO.
plugins/microshift-prow-jobs/commands/get-prow-ci-e2e-job-info.md
Outdated
Show resolved
Hide resolved
plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py
Show resolved
Hide resolved
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
agullon
force-pushed
the
microshift-prow-jobs_plugin
branch
from
0bdc57d to
22ba531
Compare
openshift-merge-robot
added
the
needs-rebase
agullon
force-pushed
the
microshift-prow-jobs_plugin
branch
from
22ba531 to
a05beda
Compare
rh-pre-commit.check-secrets: ENABLED
…elpers into microshift-prow-jobs_plugin rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
openshift-merge-robot
removed
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: agullon The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: agullon The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
@stbenjam thank you for comments. Creating and testing this plugin really helped me to understand how a LLM "is thinking", how I should ask tasks and what to expect as an output. It really shines when the task/skill is small and concise. Also, it works great, in particular when generating the output, with human-readable text (for example md files) but not with structure data (json format.). |
openshift-merge-robot
added
the
needs-rebase
WalkthroughAdds a new "microshift-prow-job" plugin: marketplace registration, plugin manifest, README, two command docs (analyze-job, analyze-test-scenario), a version-extraction skill with implementation script, and updates to docs/data.json and PLUGINS.md. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant JobCmd as /microshift-prow-job:analyze-job
participant ScenarioCmd as /microshift-prow-job:analyze-test-scenario
participant VersionSkill as extract-microshift-version
participant GCS as Prow Artifacts (GCS)
User->>JobCmd: invoke with <job-url>
JobCmd->>GCS: fetch finished.json & started.json
JobCmd->>VersionSkill: request version (job_id, job_type)
VersionSkill->>GCS: fetch build log (rf-debug.log)
VersionSkill-->>JobCmd: {success, version, build_type, url}
JobCmd->>ScenarioCmd: for each scenario -> analyze
ScenarioCmd->>GCS: fetch JUnit XML, logs, artifacts
ScenarioCmd-->>JobCmd: scenario results and artifact links
JobCmd-->>User: aggregated Markdown and JSON report
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (6 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
plugins/microshift-prow-job/README.md (2)
31-41: Add language specifiers to fenced code blocks.The code blocks at lines 31, 39 should specify language identifiers for proper syntax highlighting and markdown compliance.
Apply this pattern:
-``` +```bash /microshift-prow-job:analyze-job <job-url>--- `52-73`: **Add language specifiers to fenced code blocks.** Code blocks at lines 52, 64, 69 should specify language identifiers (likely 'bash' or 'text'). </blockquote></details> <details> <summary>plugins/microshift-prow-job/commands/analyze-job.md (1)</summary><blockquote> `1-417`: **Excellent command documentation!** The command documentation is thorough and follows the repository's man page format conventions. Implementation steps are well-detailed and include error handling scenarios. Consider adding language specifiers to fenced code blocks (e.g., 'bash', 'json', 'markdown') for better syntax highlighting and markdown compliance. </blockquote></details> <details> <summary>plugins/microshift-prow-job/commands/analyze-test-scenario.md (1)</summary><blockquote> `1-405`: **Well-documented command with good UX design!** The command documentation is thorough and the optional scenario parameter design (listing scenarios when omitted) provides good user experience. Consider adding language specifiers to fenced code blocks throughout the document for markdown compliance. </blockquote></details> <details> <summary>plugins/microshift-prow-job/skills/extract-microshift-version/SKILL.md (1)</summary><blockquote> `1-185`: **Excellent skill documentation!** The documentation thoroughly covers the skill's purpose, usage, parameters, error handling, and integration examples. Very helpful for users of the plugin. Consider adding language specifiers to the fenced code blocks at lines 154 and 172 for markdown compliance. </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro **Cache: Disabled due to data retention organization setting** **Knowledge base: Disabled due to `Reviews -> Disable Knowledge Base` setting** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between a05beda83ed0a1a83afbd103a0c4a4115180a802 and 5803e3a160e0d5b522994f2c7fefe04ca6eabee9. </details> <details> <summary>📒 Files selected for processing (8)</summary> * `.claude-plugin/marketplace.json` (1 hunks) * `PLUGINS.md` (2 hunks) * `plugins/microshift-prow-job/.claude-plugin/plugin.json` (1 hunks) * `plugins/microshift-prow-job/README.md` (1 hunks) * `plugins/microshift-prow-job/commands/analyze-job.md` (1 hunks) * `plugins/microshift-prow-job/commands/analyze-test-scenario.md` (1 hunks) * `plugins/microshift-prow-job/skills/extract-microshift-version/SKILL.md` (1 hunks) * `plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>plugins/microshift-prow-job/README.md</summary> 31-31: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 39-39: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 52-52: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 64-64: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 69-69: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 149-149: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 157-157: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 332-332: Bare URL used (MD034, no-bare-urls) </details> <details> <summary>plugins/microshift-prow-job/commands/analyze-test-scenario.md</summary> 10-10: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 91-91: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 106-106: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 114-114: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 267-267: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 279-279: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 288-288: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 302-302: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 307-307: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 341-341: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 346-346: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 361-361: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 366-366: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>plugins/microshift-prow-job/skills/extract-microshift-version/SKILL.md</summary> 154-154: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 172-172: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> <details> <summary>plugins/microshift-prow-job/commands/analyze-job.md</summary> 10-10: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 91-91: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 106-106: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 114-114: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 267-267: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 279-279: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 288-288: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 302-302: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 307-307: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 341-341: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 346-346: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 361-361: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 366-366: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 Ruff (0.14.3)</summary> <details> <summary>plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py</summary> 51-51: Audit URL open for permitted schemes. Allowing use of `file:` or custom schemes is often unexpected. (S310) --- 56-56: Do not catch blind exception: `Exception` (BLE001) </details> </details> </details> <details> <summary>🔇 Additional comments (8)</summary><blockquote> <details> <summary>plugins/microshift-prow-job/.claude-plugin/plugin.json (1)</summary><blockquote> `1-10`: **LGTM! Plugin manifest is well-structured.** The plugin manifest contains all required metadata with appropriate values for a new plugin. </blockquote></details> <details> <summary>.claude-plugin/marketplace.json (1)</summary><blockquote> `67-71`: **LGTM! Marketplace entry follows conventions.** The marketplace entry is properly formatted and consistent with other plugin entries. </blockquote></details> <details> <summary>PLUGINS.md (2)</summary><blockquote> `11-11`: **LGTM! Navigation entry added correctly.** The plugin navigation link follows the established format. --- `84-92`: **LGTM! Plugin section is well-documented.** The plugin section provides clear command descriptions and links to detailed documentation, consistent with other plugins. </blockquote></details> <details> <summary>plugins/microshift-prow-job/README.md (1)</summary><blockquote> `1-335`: **Excellent comprehensive documentation!** The README provides thorough coverage of plugin functionality, installation, usage examples, troubleshooting, and test scenario details. This will be very helpful for users. </blockquote></details> <details> <summary>plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py (3)</summary><blockquote> `33-39`: **LGTM! URL construction is clear.** The function builds GCS URLs in a straightforward manner with appropriate path components. --- `60-76`: **LGTM! Version extraction logic is sound.** The regex pattern appropriately matches the version string format in the build logs. --- `95-148`: **LGTM! Main function is well-structured.** The main function has clear flow, appropriate error handling, and consistent JSON output format. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py
Show resolved
Hide resolved
plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py
Show resolved
Hide resolved
plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py
Show resolved
Hide resolved
plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py
Outdated
Show resolved
Hide resolved
openshift-merge-robot
removed
the
needs-rebase
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
…elpers into microshift-prow-jobs_plugin rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py (1)
112-132: Add input validation to prevent malformed URLs; restrict to safe character patterns.The inputs are embedded directly into URL construction without character validation. While GCS services have access controls, defensive validation against path traversal patterns and unexpected characters is prudent. Restrict
job_id,version, andjob_typeto expected formats (alphanumeric, hyphens, underscores, dots).if not job_type or not job_type.strip(): print(json.dumps({ "success": False, "error": "job_type cannot be empty" })) sys.exit(1) + # Validate inputs against safe character set to prevent URL malformation + import string + safe_chars = set(string.ascii_letters + string.digits + '-._') + + for param_name, param_value in [("job_id", job_id), ("version", version), ("job_type", job_type)]: + if not all(c in safe_chars for c in param_value): + print(json.dumps({ + "success": False, + "error": f"{param_name} contains invalid characters. Only alphanumeric, hyphen, underscore, and dot are allowed" + })) + sys.exit(1)
🧹 Nitpick comments (1)
plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py (1)
45-61: Good improvements on URL validation and exception handling.The addition of URL scheme validation (lines 45-47) and specific exception handlers (lines 60-61) addresses previous security concerns. The SSL certificate verification is intentionally disabled for internal CI environments, which is acceptable.
Minor improvement:
Apply this diff to remove unnecessary f-string prefix:
if not url.startswith('https://'): - return None, f"Invalid URL scheme. Only HTTPS URLs are supported." + return None, "Invalid URL scheme. Only HTTPS URLs are supported."Based on learnings
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
docs/data.json(1 hunks)plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.3)
plugins/microshift-prow-job/skills/extract-microshift-version/extract_microshift_version.py
47-47: f-string without any placeholders
Remove extraneous f prefix
(F541)
55-55: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
🔇 Additional comments (1)
docs/data.json (1)
561-587: LGTM! Plugin registration is well-structured.The new microshift-prow-job plugin registration follows the established pattern of other plugins in the repository, with properly defined commands and skills.
|
@stbenjam can you please review this again? |
|
@stbenjam I was thinking probably it makes more sense to add these commands into the microshift repo itself. Because they only target test executed from this repo and also other team members are adding more comments. Do you mind I close this PR and open another in the microshift repo? |
|
I think it makes more sense to add these 2 commands in microshift repository, so I'll close this PR in favor of openshift/microshift#5754 PR. |
3 participants
What this PR does / why we need it:
This PR adds a new Claude Code plugin for analyzing MicroShift Prow CI job executions and test results. The microshift-prow-job plugin provides automated analysis tools for MicroShift CI jobs running on OpenShift's Prow CI infrastructure. It helps developers and QE engineers quickly understand test results, diagnose failures, and track MicroShift versions across different test configurations.
Commands Added:
/microshift-prow-job:analyze-job <job-url>/microshift-prow-job:analyze-test-scenario <job-url> [scenario-name]Special notes for your reviewer:
Summary by CodeRabbit
New Features
Documentation